Skip to content

Figure.logo: Let the 'style' parameter support descriptive arguments #4074

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 19 commits into from
Sep 18, 2025
Merged

Figure.logo: Let the 'style' parameter support descriptive arguments #4074

merged 19 commits into from
Sep 18, 2025

Conversation

seisman
Copy link
Member

@seisman seisman commented Sep 3, 2025 •
edited
Loading

This PR improves the style parameter (i.e., -S option). Previously the style parameter can only accept l/n/u. Now it can accept more descriptive arguments standard/no_label/url.

The upstream GMT uses long names standard/none/url (see https://github.com/GenericMappingTools/gmt/blob/c95470b1c78c412fa15d388be0d9b07de111ba49/src/longopt/gmtlogo_inc.h#L37), but I feel no_label is more descriptive.

Preview: https://pygmt-dev--4074.org.readthedocs.build/en/4074/api/generated/pygmt.Figure.logo.html

@seisman seisman added this to the 0.17.0 milestone Sep 3, 2025
@seisman seisman requested review from a team and Copilot September 3, 2025 14:23
@seisman seisman added enhancement Improving an existing feature needs review This PR has higher priority and needs review. labels Sep 3, 2025
Copilot
Copilot AI reviewed Sep 3, 2025
View reviewed changes
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR enhances the style parameter in the Figure.logo method to accept more descriptive string arguments instead of single-letter codes. The change improves code readability by allowing "standard", "url", and "no_label" instead of cryptic "l", "u", and "n" values.

  • Updated the style parameter to use descriptive literal types with proper type hints
  • Implemented alias mapping to convert descriptive names to GMT's single-letter codes
  • Updated documentation to reflect the new descriptive parameter values

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@weiji14
Copy link
Member

weiji14 commented Sep 3, 2025

Hmm, I'm getting a 403 when trying to access https://dagshub.com/GenericMappingTools/pygmt, server might be down or something.

@seisman
Copy link
Member Author

seisman commented Sep 4, 2025

Hmm, I'm getting a 403 when trying to access https://dagshub.com/GenericMappingTools/pygmt, server might be down or something.

I can access the website, and I can even run dvc pull successfully using my local computer. So it's more likely CI issues.

@yvonnefroehlich
Copy link
Member

Hmm, I'm getting a 403 when trying to access https://dagshub.com/GenericMappingTools/pygmt, server might be down or something.

I can access the website, and I can even run dvc pull successfully using my local computer.

Same for me.

@yvonnefroehlich
Copy link
Member

The upstream GMT uses long names standard/none/url (see https://github.com/GenericMappingTools/gmt/blob/c95470b1c78c412fa15d388be0d9b07de111ba49/src/longopt/gmtlogo_inc.h#L37), but I feel no_label is more descriptive.

I prefere no_label. In general, I feel having "none" and None is not optimal.

yvonnefroehlich
yvonnefroehlich reviewed Sep 4, 2025
View reviewed changes
seisman and others added 3 commits September 4, 2025 18:29
@seisman @yvonnefroehlich
Fix site to website
5f981c5 
Co-authored-by: Yvonne Fröhlich <[email protected]>
@dependabot @seisman
Build(deps): Bump codecov/codecov-action from 5.4.3 to 5.5.0 (#4076)
1741d0a 
Bumps [codecov/codecov-action](https://github.com/codecov/codecov-action) from 5.4.3 to 5.5.0.
- [Release notes](https://github.com/codecov/codecov-action/releases)
- [Changelog](https://github.com/codecov/codecov-action/blob/main/CHANGELOG.md)
- [Commits](codecov/codecov-action@18283e0...fdcc847)

---
updated-dependencies:
- dependency-name: codecov/codecov-action
  dependency-version: 5.5.0
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@seisman
Merge branch 'main' into logo/style
eea5ae4
@seisman
Copy link
Member Author

seisman commented Sep 6, 2025

I tried debugging the DVC issue but wasn't successful. I think we'll have to move on for now and revisit it in the future if a fix becomes available.

@seisman seisman requested a review from a team September 10, 2025 12:05
@seisman seisman added final review call This PR requires final review and approval from a second reviewer and removed needs review This PR has higher priority and needs review. labels Sep 13, 2025
@seisman
Copy link
Member Author

seisman commented Sep 15, 2025

Ping @GenericMappingTools/pygmt-maintainers for final reviews. Will merge in 24 hours.

yvonnefroehlich
yvonnefroehlich reviewed Sep 15, 2025
View reviewed changes
Copy link
Member

@yvonnefroehlich yvonnefroehlich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel style was introduced to consistently use this alias for -S. But I am wondering if something linke label would be more suitable here.

@seisman
Copy link
Member Author

seisman commented Sep 15, 2025

I feel style was introduced to consistently use this alias for -S.

Yes.

But I am wondering if something linke label would be more suitable here.

label also sounds a good name, but we already have label aliased to the common option -l. Using label here may be a little misleading.

@seisman
Copy link
Member Author

seisman commented Sep 17, 2025

Any more comments on the changes? Plan to merge it in 24 hours.

weiji14
weiji14 approved these changes Sep 17, 2025
View reviewed changes
@seisman seisman removed the final review call This PR requires final review and approval from a second reviewer label Sep 18, 2025
@seisman seisman merged commit 88dc47b into main Sep 18, 2025
16 of 21 checks passed
@seisman seisman deleted the logo/style branch September 18, 2025 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yvonnefroehlich yvonnefroehlich yvonnefroehlich approved these changes

Copilot code review Copilot Copilot left review comments

@weiji14 weiji14 weiji14 approved these changes

Assignees
No one assigned
Labels
enhancement Improving an existing feature
Projects
None yet
Milestone
0.17.0
Development

Successfully merging this pull request may close these issues.
3 participants
@seisman @weiji14 @yvonnefroehlich